Skip to content

Recognize the MS-DIAL 5 suggested-annotation name prefixes - #784

Merged
YukiMatsuzawa merged 2 commits into
masterfrom
fix/msdial5-suggested-annotation-name-predicate
Sep 4, 2026
Merged

Recognize the MS-DIAL 5 suggested-annotation name prefixes#784
YukiMatsuzawa merged 2 commits into
masterfrom
fix/msdial5-suggested-annotation-name-predicate

Conversation

@htsugawa

@htsugawa htsugawa commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem

MS-DIAL 4 marked its precursor-only suggestions with a single "w/o MS2:" prefix. MS-DIAL 5 splits that bucket in two, in DataAccess.SetMoleculeMsPropertyAsSuggested:

shape meaning
no MS2: X MS2RawSpectrumID < 0 — no product-ion spectrum at all
low score: X a product-ion spectrum exists but the reference search did not meet the acceptance criteria

The "w/o MS2: " line in SetMoleculeMsProperty is commented out. Two readers still tested only the MS-DIAL 4 spelling, which never matches MS-DIAL 5 output:

  1. MoleculerNetworking.GetOntologyColor gave precursor-only and low-score suggestions an ontology colour on the molecular network, identical to a confident annotation — on a graph whose edges are built from MS/MS similarity, so a miscoloured node seeds false annotation propagation by eye. Reachable from MsdialGuiApp only; the Console msn command uses the edge-only statics and does not build nodes.
  2. DataAccess.IsReferenceMatchedName (added in Feature/console retention time correction #766) reported them as Reference matched in the Console quality-assurance matrix. This is the Console path MS-DIAL Interactive drives.

Measured on the checked-out console_fastlc_demo output: the .mdalign holds 1021 rows starting no MS2: , 230 starting low score: , and zero starting w/o MS2.

Decision

Both shapes are excluded. They are produced only on the branches where MsScanMatchResult.IsReferenceMatched is false (MsScanMatching sets it as IsSpectrumMatch && TotalScore > cutoff && RT/RI match), so neither is an accepted reference match. A low-score row does have product-ion evidence and ranks above a precursor-only one, but the ontology colour and the Reference matched flag are both binary claims about an accepted annotation, and neither shape qualifies. Preserving the low-score/precursor-only distinction visually would be a third state — separate work, not this predicate.

"w/o MS2: " is kept: SetPeptideMsPropertyAsSuggested still writes it, and MS-DIAL 4 wrote it everywhere. The MS-DIAL 4 sources under src/MSDIAL4/ are correct as they stand and are untouched.

Change

The prefix vocabulary and the predicate now live together in CompMs.Common.Utility.AnnotationName, and the writers build their names from it, so a renamed prefix cannot leave a reader stale. The helper is in CommonStandard because the vocabulary is shared by CommonStandard (networking) and MsdialCore (export) and the project dependency runs MsdialCore → CommonStandard only. DataAccess.IsReferenceMatchedName is kept as a delegating overload.

One behavioural refinement beyond the added prefixes: GetOntologyColor moves from Contains to the helper's StartsWith-after-TrimStart matching, so a legitimate compound name that merely contains Unknown or RIKEN mid-string is no longer misread. All MS-DIAL 5 shapes are prefixes. IsReferenceMatchedName already used StartsWith, so for the Console path the change is purely additive.

Verification

  • CommonStandardTests 844/844 pass; MsdialCoreTests 295/295; MsdialLcMsApiTests 66 pass / 2 skipped.
  • New tests cover all four MS-DIAL 5 name shapes plus the peptide shape and the placeholders, and assert the writer/reader invariant. With the two prefixes removed from the predicate they fail as Expected:<rgb(0,0,0)>. Actual:<rgb(255, 165, 0)>, reproducing the reported symptom.
  • MSDIALCUI built Release/net48 (5.5.241113) and the console_fastlc_demo LC-MS run re-executed over its 7 SCIEX WIFF files, exit 0.
  • .mdalign byte-identical to the pre-fix run. .mzTab identical once the job id is normalized. In .qa.tsv, columns 1–11 are byte-identical and column 12 changes in one direction only: 3919 rows TRUE → FALSE, 0 FALSE → TRUE, with the 1196 genuine TRUE rows preserved.

Follow-up, deliberately not in this PR

MztabFormatExport.cs (lines 172, 309, 495) tests Contains("no MS2") and so already excludes precursor-only suggestions, but not low score. Whether low-score rows belong in mzTab-M is a separate scientific question about that export, so the exporter is untouched here — including its string literals.

MoleculerSpectrumNetworkingTest.MergeNodeFiles in the Console test app carries the same stale literal, but its only caller is commented out, so it has no behavioural effect.

🤖 Generated with Claude Code

MS-DIAL 4 marked its precursor-only suggestions with a single "w/o MS2:"
prefix. MS-DIAL 5 splits that bucket in two: SetMoleculeMsPropertyAsSuggested
writes "no MS2: " when the feature has no product-ion spectrum
(MS2RawSpectrumID < 0) and "low score: " when a product-ion spectrum exists
but the reference search did not meet the acceptance criteria. Two readers
still tested only the MS-DIAL 4 spelling, which never matches MS-DIAL 5
output, so both suggestion shapes were treated as accepted annotations:

- MoleculerNetworking.GetOntologyColor gave precursor-only and low-score
  suggestions an ontology colour on the molecular network, identical to a
  confident annotation, on a graph whose edges are MS/MS similarity.
- DataAccess.IsReferenceMatchedName reported them as Reference matched in
  the Console quality-assurance matrix.

Both name shapes are produced only on the branches where
MsScanMatchResult.IsReferenceMatched is false, so neither is an accepted
reference match. "w/o MS2: " is kept: SetPeptideMsPropertyAsSuggested still
writes it.

The prefix vocabulary and the predicate now live together in
CompMs.Common.Utility.AnnotationName, so a renamed prefix cannot leave a
reader stale, and the writers build their names from it. The helper is in
CommonStandard because the vocabulary is shared by CommonStandard and
MsdialCore and the project dependency runs MsdialCore -> CommonStandard.

Verified on the console_fastlc_demo LC-MS run (7 SCIEX WIFF files): mdalign
and mzTab-M output are byte-identical, and the quality-assurance matrix
changes in one direction only, 3919 rows TRUE -> FALSE and none the other
way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Newly added test files are missing required using directives, which will prevent the test projects from compiling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR centralizes MS-DIAL “suggested annotation” name prefixes and the “is this an accepted reference match?” predicate so that both MS-DIAL 5 suggestion shapes (no MS2: / low score:) are consistently excluded by downstream readers (molecular networking node coloring and Console QA export).

Changes:

  • Added CompMs.Common.Utility.AnnotationName to own suggestion-prefix vocabulary and the IsReferenceMatched() predicate.
  • Updated MS-DIAL 5 name writers (DataAccess.Set*M*AsSuggested) and readers (DataAccess.IsReferenceMatchedName, MoleculerNetworking.GetOntologyColor) to use AnnotationName.
  • Added regression tests to cover MS-DIAL 5 suggestion prefixes and the writer/reader invariant.
File summaries
File Description
tests/MSDIAL5/MsdialCoreTests/Utility/DataAccessTests.cs Extends QA/export test cases to include MS-DIAL 5 suggestion prefixes.
tests/Common/CommonStandardTests/Utility/AnnotationNameTests.cs New unit tests for the centralized suggestion-prefix predicate and writers.
tests/Common/CommonStandardTests/Algorithm/Function/MoleculerNetworkingTests.cs New regression tests ensuring suggestion-shaped names do not receive ontology coloring.
src/MSDIAL5/MsdialCore/Utility/DataAccess.cs Uses AnnotationName for suggestion naming and delegates IsReferenceMatchedName() to it.
src/Common/CommonStandard/Utility/AnnotationName.cs New shared helper defining prefixes and the IsReferenceMatched() predicate.
src/Common/CommonStandard/Algorithm/Function/MoleculerNetworking.cs Replaces ad-hoc Contains(...) checks with AnnotationName.IsReferenceMatched(...).
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@@ -0,0 +1,78 @@
using CompMs.Common.Components;
Comment on lines +1 to +2
using Microsoft.VisualStudio.TestTools.UnitTesting;

@YukiMatsuzawa
YukiMatsuzawa merged commit 500baa0 into master Sep 4, 2026
9 checks passed
@YukiMatsuzawa
YukiMatsuzawa deleted the fix/msdial5-suggested-annotation-name-predicate branch September 4, 2026 07:05
htsugawa added a commit that referenced this pull request Sep 5, 2026
* Export alignment spectrum provenance links

* Add compact alignment peak ID matrix export

* Stop the provenance sidecar asserting what the run did not establish

Review of the rebased branch found the audit sidecar reporting three things that
were not measurements, all of them invisible to the existing tests because both
tests asserted substrings of a row that had a real source peak, and stopped
before the mz column.

The mz column was read from ChromXsTop.Mz in the in-memory overload and from
Mass in the light overload. On an aligned peak ChromXsTop carries the
chromatogram axis, and the gap filler resets it outright, so the column reported
a sentinel for every member that DID have a source peak and a real value only
for the gap-filled ones. Measured on the FastLC demo: 7879 of 7879 rows with
has_source_peak=true carried mz=-1, and 9992 of 9992 gap-filled rows carried a
plausible m/z. Exactly backwards, in every row. Both overloads now read Mass,
which is also what the .mdalign MZ column uses.

A member with no source peak has no source spectrum either, but the gap filler
leaves the spectrum ids at their default 0 while resetting only the peak ids, so
every gap-filled row published ms1_raw_spectrum_id=0, ms1_raw_spectrum_id_top=0
and ms2_raw_spectrum_id=0. A 0 is a real scan index. In a file whose whole
purpose is provenance, that points an auditor at a spectrum belonging to some
other peak. All five raw-spectrum columns are now withheld when there is no
source peak, in both overloads; the light overload had additionally gated
ms2_raw_spectrum_ids on MS2RawSpectrumID >= 0, which is true for that same 0.

The peak-id columns published -2 for a gap-filled cell while the compact peak-ID
matrix published -1 for the same cell, so two files shipped by one feature
disagreed numerically. They now agree on -1, and the distinction the raw values
carried is preserved by name in a new peak_origin column, valued detected,
gap_filled or absent. That is the same lesson as PR #785: name the state rather
than encode it in a magic number, so a reader does not need the gap filler's
internals to interpret a cell.

Also fix the light-mode QA matrix's "Reference matched" column. The switch to
the file-backed quant accessor made light mode derive it from
RepresentativeLibraryID >= 0, but a suggested annotation carries a library id
too, so a precursor-only or low-score suggestion counted as a reference match in
light mode while normal mode reported false. Normal mode resolves the column
through AnnotationName.IsReferenceMatched, the predicate PR #784 centralised.
The store now persists the representative's own IsReferenceMatched verdict and
the accessor reads that. RecordSize goes 102 to 103; the file is a per-run
temporary spill created by Path.GetTempFileName, so the layout carries no
cross-version obligation.

Two guards so this class of defect cannot return silently. WriteMember now goes
through a WriteRow that refuses a field count the header does not declare, since
the header and the two value lists are three hand-maintained arrays. And the
tests assert whole lines rather than substrings, and cover a gap-filled row and
an unwritten light row in both overloads, which is the case that hid all three
divergences.

Build: Release net48, 0 errors. Tests: MsdialCoreTests 316/316 (311 before, plus
five new), MsdialCoreTestAppTests 13/13 including a new
ReadDetailedAlignmentProvenance test alongside master's ReadLbmAnnotatorPriority
test in the project that actually compiles the rebased ConfigParser.

Verified end to end on the FastLC demo, ingesting the output into the companion
msdial_spectrum_catalog: every member with a source peak now carries a usable
m/z, no member without one carries a scan index, and peak_origin splits the
17871 members into 7879 detected and 9992 gap_filled.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: DESKTOP-382ETUR\Hiroshi Tsugawa <htsugawa@go.tuat.ac.jp>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants